Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade pull-ws #50

Merged
merged 8 commits into from
Apr 25, 2021
Merged

Upgrade pull-ws #50

merged 8 commits into from
Apr 25, 2021

Conversation

dominictarr
Copy link
Contributor

These changes are necessary after recent changes to pull-stream/pull-ws#26

I also improved test coverage, reusing the same tests for net and ws. @mixmix

@stale
Copy link

stale bot commented Sep 7, 2019

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

@stale stale bot added the stale label Sep 7, 2019
@dominictarr
Copy link
Contributor Author

@christianbundy also this

@stale stale bot removed the stale label Sep 18, 2019
@dominictarr
Copy link
Contributor Author

@christianbundy I think maybe this just needs to be merged. check if the tests of sbot and multiserver still work when this is linked in.

@dominictarr
Copy link
Contributor Author

if it does then hit merge

@christianbundy christianbundy self-assigned this Sep 18, 2019
@christianbundy
Copy link
Contributor

This branch isn't passing tests for me locally, and Travis is reporting the same problem. It seems to be hanging here:

# combined.ws, aborted

server error, from ws:::ffff:127.0.0.1~shs:

Error: stream ended with:0 but wanted:64

    at drain (/home/travis/build/ssbc/multiserver/node_modules/pull-reader/index.js:43:26)

    at /home/travis/build/ssbc/multiserver/node_modules/pull-reader/index.js:63:18

    at /home/travis/build/ssbc/multiserver/node_modules/pull-reader/index.js:20:7

    at WebSocket.<anonymous> (/home/travis/build/ssbc/multiserver/node_modules/pull-ws/source.js:40:7)

    at WebSocket.onClose (/home/travis/build/ssbc/multiserver/node_modules/ws/lib/WebSocket.js:446:14)

    at WebSocket.emit (events.js:203:15)

    at WebSocket.cleanupWebsocketResources (/home/travis/build/ssbc/multiserver/node_modules/ws/lib/WebSocket.js:950:8)

    at Socket.emit (events.js:203:15)

    at endReadableNT (_stream_readable.js:1129:12)

    at process._tickCallback (internal/process/next_tick.js:63:19)

@arj03
Copy link
Member

arj03 commented Apr 6, 2020

It works if you link in the upgraded pull-ws

@christianbundy christianbundy changed the base branch from master to main August 25, 2020 20:24
@arj03
Copy link
Member

arj03 commented Sep 1, 2020

Merge with main. Waiting for pull-ws to be merged to be able to move forward. After that is in, the tests for this should be working again.

@arj03
Copy link
Member

arj03 commented Dec 21, 2020

It seems like pull-ws is dead. Nobody with commit access seems interested. I'm considering just forking it and using that for my projects, and update multiserver to use the fork. Any thoughts on this?

@christianbundy
Copy link
Contributor

sounds great to me

@mixmix
Copy link
Member

mixmix commented Mar 9, 2021

I'm in support of forking too @arj03

@arj03
Copy link
Member

arj03 commented Mar 9, 2021

Ah right. Had forgotten about this one completely. Thanks for the ping :-)

@arj03
Copy link
Member

arj03 commented Apr 22, 2021

Alright so I think this is ready. I forked pull-ws as pull-websocket and fixed the tests. With this I have had a quite stable room connection with patchwork. @cryptix shall we test rooms2 tomorrow? :)

@staltz
Copy link
Member

staltz commented Apr 23, 2021

Cool, I'd like to merge it but gotta fix some git conflicts first

@arj03
Copy link
Member

arj03 commented Apr 23, 2021

The conflicts should be fixed, the only thing I'd maybe like to fix is this line. That test was not in the old code so I don't think it's a new problem.

@arj03
Copy link
Member

arj03 commented Apr 23, 2021

Alrighty part2: I fixed the tests now as well (mainly in pull-websocket). When you call close on a server it will only stop accepting new connections, not actually kill existing. IIRC the argument for that is related to deadlocks and its a common gotcha.

@arj03
Copy link
Member

arj03 commented Apr 25, 2021

Does your comment @staltz mean that you are okay if I merge this? :)

@staltz
Copy link
Member

staltz commented Apr 25, 2021

Yes!

@arj03 arj03 merged commit 598178a into main Apr 25, 2021
@arj03 arj03 deleted the upgrade-ws branch April 25, 2021 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants